Skip to content

fix(deploy): inject DOCKER_BUILD_ARGS into the Dockerfile#118

Merged
bacongobbler merged 1 commit into
deis:masterfrom
bacongobbler:buildargs
Mar 2, 2017
Merged

fix(deploy): inject DOCKER_BUILD_ARGS into the Dockerfile#118
bacongobbler merged 1 commit into
deis:masterfrom
bacongobbler:buildargs

Conversation

@bacongobbler
Copy link
Copy Markdown
Member

this should close deis/builder#490.

@felixbuenemann
Copy link
Copy Markdown

@bacongobbler Won't this cause a rebuild of the docker image every time a new environment var is set on the app? Or is docker smart enough about that?

@bacongobbler
Copy link
Copy Markdown
Member Author

If a Dockerfile defines an ARG variable whose value is different from a previous build, then a “cache miss” occurs upon its first usage, not its definition. Since it's appended to the end of the Dockerfile then this won't cause cache misses.

@vdice
Copy link
Copy Markdown
Member

vdice commented Mar 2, 2017

@bacongobbler may need a minor formatting change to add a newline between multiple args:

$ deis config:set PORT=7777 DOO=FOO -a edh
Creating config... done

=== edh Config
DOO       FOO
PORT      7777

$ git push edh-mk master
...
Step 12 : ARG DOOARG PORT
ARG requires exactly one argument definition

@bacongobbler
Copy link
Copy Markdown
Member Author

Thanks for testing this! I even ensured that we write to a new line as well to ensure we're not clashing with existing dockerfiles.

@vdice
Copy link
Copy Markdown
Member

vdice commented Mar 2, 2017

Testing looks good; happy path spec around this added in deis/workflow-e2e#349 (didn't appear we previously had a spec that covered config:set on dockerfile app). The e2e run on that pr will run with this change as well.

@vdice vdice added the LGTM1 label Mar 2, 2017
@bacongobbler bacongobbler merged commit 8eb4428 into deis:master Mar 2, 2017
@bacongobbler bacongobbler deleted the buildargs branch March 2, 2017 22:00
Comment thread rootfs/deploy.py
# ensure we are on a new line
dockerfile.write("\n")
for envvar in buildargs:
dockerfile.write("ARG {}\n".format(envvar))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the with block can be limited to float(client.version()['ApiVersion']) < 1.13 because the error was changed to a warning in moby/moby#26249 which is part of the 1.13.0 milestone (unless you also want to suppress the warning).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

config:set no longer appears to work for Dockerfile-based apps

4 participants